Fix deserialization of unions #332

forest-benchling · 2021-02-09T22:24:45Z

Deserialization of unions in from_dict was not working properly, because it failed to account for cases where the union property is None or Unset. This PR adds explicit checks for None or Unset.

In addition, the type of the value to deserialize was previously set to Any, which was masking type errors. In order to fix these type errors, I introduced a new concept of check_type_for_construct for each property, which allows the type to be checked before attempting deserialization.

In the future, I think we should refine the try/except pattern; it's not good practice to rely on a catch-all except when trying to deserialize each type within the union, because we don't know what exception was actually raised.

As part of this PR, I also refactored the type_string method to reuse logic across properties. Also as part of this PR, I had include #335, which had a circular dependency on this PR:

Previously, if you had a oneOf of inlined models, each inline model was generated with the same name, causing effectively only the last model to be included (since it overwrote all the previous models).

In this PR, we give it an autogenerated name, to avoid the name clash.

forest-benchling · 2021-02-09T23:08:32Z

@dbanty I opened a new PR, since the git history and merge conflicts with the template renaming (adding jinja) was too hard to reconcile.

codecov · 2021-02-09T23:08:34Z

Codecov Report

Merging #332 (0d90970) into main (2861a34) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #332   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1410      1420   +10     
=========================================
+ Hits          1410      1420   +10

Impacted Files	Coverage Δ
...penapi_python_client/parser/properties/__init__.py	`100.00% <100.00%> (ø)`
...i_python_client/parser/properties/enum_property.py	`100.00% <100.00%> (ø)`
..._python_client/parser/properties/model_property.py	`100.00% <100.00%> (ø)`
...penapi_python_client/parser/properties/property.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2861a34...0d90970. Read the comment docs.

dbanty

Looks good, nice to see the None/Unset type string refactor make it back in! I have a few suggestions and Codecov seems to be saying we're missing a unit test somewhere.

dbanty · 2021-02-10T16:35:56Z

end_to_end_tests/golden-record-custom/custom_e2e/models/a_model.py

+            not_required_one_of_models = UNSET
+            if not isinstance(self.not_required_one_of_models, Unset):
+                not_required_one_of_models = self.not_required_one_of_models.to_dict()


Will have to check later why this is the case. I assume there's some difficulty with handling the type in the template which is why we're checking multiple isinstances here.

This is the OpenAPI:

"not_required_one_of_models": { "oneOf": [ { "ref": "#components/schemas/FreeFormModel" }, { "ref": "#components/schemas/ModelWithUnionProperty" } ], "nullable": false },

My guess is that the inner properties are not required? Kind of confusing what the semantics should be...

dbanty · 2021-02-10T16:38:03Z

end_to_end_tests/golden-record-custom/custom_e2e/models/a_model.py

+                if not isinstance(data, str):
+                    raise TypeError()
                a_camel_date_time = isoparse(data)


I think letting isoparse raise the error (if we're going to raise it anyway) would be better because it would give more context as to what is wrong. It's especially confusing to readers to see this since the input type of the function is str, so why would this not be one?

Oh I see, this is here to get the correct Union option more easily? Maybe then we need to exclude it if no Union? Not really necessary for this PR, there are plenty of things like this that need cleanup at some point.

Ah, yeah, this is just a special case when there's a union of only 1 element. I think it's pretty unlikely to happen in actual usage. Let me try to fix it though.

Turns out to be unnecessary if we're changing the input type to be object.

dbanty · 2021-02-10T16:41:28Z

end_to_end_tests/golden-record-custom/custom_e2e/models/a_model.py

+                if not isinstance(data, dict):
+                    raise TypeError()
+                one_of_models = FreeFormModel.from_dict(data)


Same here re: confusion. If we can't be confident in the data passed in, the type annotation should be : Any and we do the runtime check. If we are confident of what's passed in, we can do the correct type annotation and not a runtime check.

I think the only reason this would be the wrong data type is if the OpenAPI spec was wrong or someone made a breaking change to the API. Either way we can't properly handle all the cases that could fail. So I think just the proper type annotation without the runtime check is probably the better approach?

Yeah, that's a good point. Actually initially I didn't have a runtime check, but then MyPy throws an error, because it's not type-safe.

I'm starting to think that maybe the original Any annotation is better, since we can't be confident in the data passed in. But I think it should be object, not Any, because Any causes type errors to be ignored, whereas object will force us to be type-safe.

Sounds good regarding object vs Any if that ends up working better.

dbanty · 2021-02-10T16:44:04Z

end_to_end_tests/golden-record-custom/custom_e2e/models/a_model.py

        nullable_model = None
        _nullable_model = d.pop("nullable_model")
        if _nullable_model is not None:
-            nullable_model = AModelNullableModel.from_dict(cast(Dict[str, Any], _nullable_model))
+            nullable_model = AModelNullableModel.from_dict(_nullable_model)


I'm surprised mypy doesn't get mad about us passing an Any to a function which takes Dict[str, Any]

Yeah, it's because Any has weird semantics, in that it basically just means "ignore any type errors". If we were to change the signature to object, then it would error.

dbanty · 2021-02-10T16:55:37Z

openapi_python_client/parser/properties/__init__.py

+    def _get_inner_type_strings(self, json: bool = False) -> List[str]:
+        inner_types = [p.get_type_string(no_optional=True, json=json) for p in self.inner_properties]
+        unique_inner_types = list(dict.fromkeys(inner_types))
+        return unique_inner_types


Suggested change

def _get_inner_type_strings(self, json: bool = False) -> List[str]:

inner_types = [p.get_type_string(no_optional=True, json=json) for p in self.inner_properties]

unique_inner_types = list(dict.fromkeys(inner_types))

return unique_inner_types

def _get_inner_type_strings(self, json: bool = False) -> Set[str]:

return {p.get_type_string(no_optional=True, json=json) for p in self.inner_properties}

Ah, I think I tried this, but IIRC it was causing the codegen to be non-deterministic 😬 (or at least unpredictable)

I see. We could, when actually joining them (I think that's the only place they are used for output?) add a sorted to make it deterministic so it passes e2e tests?

dbanty · 2021-02-10T17:00:35Z

openapi_python_client/parser/properties/__init__.py

+    def get_type_strings_in_union(self, no_optional: bool = False, json: bool = False) -> List[str]:
+        type_strings = self._get_inner_type_strings(json=json)
+
+        if no_optional or (self.required and not self.nullable):
+            return type_strings
+        elif self.required and self.nullable:
+            return ["None"] + type_strings
+        elif not self.required and self.nullable:
+            return ["Unset", "None"] + type_strings
+        else:
+            return ["Unset"] + type_strings


Suggested change

def get_type_strings_in_union(self, no_optional: bool = False, json: bool = False) -> List[str]:

type_strings = self._get_inner_type_strings(json=json)

if no_optional or (self.required and not self.nullable):

return type_strings

elif self.required and self.nullable:

return ["None"] + type_strings

elif not self.required and self.nullable:

return ["Unset", "None"] + type_strings

else:

return ["Unset"] + type_strings

def get_type_strings_in_union(self, no_optional: bool = False, json: bool = False) -> Set[str]:

type_strings = self._get_inner_type_strings(json=json)

if no_optional:

return type_strings

if self.nullable:

type_strings.add("None")

if not self.required:

type_strings.add("Unset")

return type_strings

(Same as above, concern about non-determinism)

dbanty · 2021-02-10T17:08:39Z

openapi_python_client/templates/property_templates/enum_property.py.jinja

    {{ property.python_name }} = {{ property.reference.class_name }}(_{{ property.python_name }})
 {% endif %}
 {% endmacro %}

+{% macro check_type_for_construct(property, source) %}{% if property.value_type == str %}isinstance({{ source }}, str){% else %}isinstance({{ source }}, int){% endif %}{% endmacro %}


Suggested change

{% macro check_type_for_construct(property, source) %}{% if property.value_type == str %}isinstance({{ source }}, str){% else %}isinstance({{ source }}, int){% endif %}{% endmacro %}

{% macro check_type_for_construct(property, source) %}isinstance({{ source }}, {{ property.value_type }}){% endmacro %}

dbanty · 2021-02-10T17:14:38Z

openapi_python_client/templates/types.py.jinja

@@ -11,6 +11,9 @@ class Unset:

 UNSET: Unset = Unset()

+# Used as `FileProperty._json_type_string`


Suggested change

# Used as `FileProperty._json_type_string`

{# Used as `FileProperty._json_type_string` #}

No reason to have that comment make it to generated code I think

forest-benchling · 2021-02-10T23:32:36Z

I believe the failing test will be fixed by https://github.com/triaxtec/openapi-python-client/pull/335/files#diff-23d4c94f056bacffca42533cd744b9df1851d32b1947472bc1683f52cf977da6 (🤯 )

dbanty · 2021-02-10T23:51:51Z

openapi_python_client/parser/properties/__init__.py

+        if len(inner_types) == 1:
+            return inner_types[0]
+        else:
+            return f"Union[{', '.join(inner_types)}]"


Suggested change

return f"Union[{', '.join(inner_types)}]"

return f"Union[{', '.join(sorted(inner_types))}]"

I think this will fix the determinism issue? While also maintaining the performance and reduced code of using sets.

forest-benchling · 2021-02-11T14:18:22Z

@dbanty Sorry, I had to lump #335 into this PR, because of a circular dependency. By changing adding an explicit type check in the parsing code, this PR exposes a type error fixed by #335, while #335 depends on fixing the JSON type as done in this PR.

dbanty

Looking good, I think the only required change is to cover the rest of the union type functions with unit tests.

dbanty · 2021-02-11T18:21:58Z

openapi_python_client/parser/properties/__init__.py

+            return inner_types.pop()
+        else:
+            return f"Union[{', '.join(sorted(inner_types))}]"
+
+    def get_base_type_string(self) -> str:
+        return self._get_type_string_from_inner_type_strings(self._get_inner_type_strings(json=False))
+
+    def get_base_json_type_string(self) -> str:
+        return self._get_type_string_from_inner_type_strings(self._get_inner_type_strings(json=True))


Missing code coverage on a couple of these lines

dbanty · 2021-02-11T18:23:42Z

openapi_python_client/parser/properties/__init__.py

        sub_prop, schemas = property_from_data(
-            name=name, required=required, data=sub_prop_data, schemas=schemas, parent_name=parent_name
+            name=f"{name}_item{i}", required=required, data=sub_prop_data, schemas=schemas, parent_name=parent_name


Personally "item" makes me think of a collection (e.g. list) and I think "type" might be more clear? But that's very subjective so up to you.

Also, we should probably set required=True here. I think that would solve our extra Unset checks in the generated code. Really either the whole Union is required / nullable or None of it is. Not up to the individual types I don't think.

Would changing this cause any other problems you think?

That sounds reasonable, but is it OK if we punt changing required=True to a future PR, to keep the scope of this PR limited, unless it's necessary here?

dbanty · 2021-02-11T18:25:16Z

openapi_python_client/templates/property_templates/model_property.py.jinja

@@ -8,6 +8,8 @@
 {{ construct_template(construct_function, property, source, initial_value=initial_value) }}
 {% endmacro %}

+{% macro check_type_for_construct(property, source) %}isinstance({{ source }}, dict){% endmacro %}
+
 {% macro transform(property, source, destination, declare_type=True) %}


We probably need a better name for this macro than "transform" at some point 😅even I can't remember what this one is for sometimes. Not a note on this PR, just a note on how I need to put some design thought into the templates.

Haha, yeah...maybe serialize and deserialize?

or to_json and from_json...

dbanty · 2021-02-11T18:29:18Z

openapi_python_client/templates/property_templates/union_property.py.jinja

+    {% if "None" in property.get_type_strings_in_union(json=True) %}
+    if data is None:
+        return data
+    {% endif %}
+    {% if "Unset" in property.get_type_strings_in_union(json=True) %}
+    if isinstance(data, Unset):
+        return data
+    {% endif %}


I think we can use property.required and property.nullable for this? Maybe that's not robust enough, not sure.

I think that depends on the unrolling unions thing you had mentioned below, since a nested property could still be nullable. And technically I guess you could have a none-type in the union.

dbanty · 2021-02-11T18:32:19Z

openapi_python_client/templates/property_templates/union_property.py.jinja

-    {% from "property_templates/" + inner_property.template import construct %}
+        {{ inner_property.python_name }}: {{ inner_property.get_type_string() }}
+    {% from "property_templates/" + inner_property.template import construct, check_type_for_construct %}
+        if not {{ check_type_for_construct(inner_property, "data") }}:


In theory we could start doing:

if {{ check_type_for_construct(inner_property, "data") }}: {{ construct(inner_property, "data", initial_value="UNSET") }} return {{ inner_property.python_name }}

and avoid the try/except stuff, yeah?

I don't think it would work out-of-the-box, because we're also catching non-type errors, like parsing dates (isoparse("foo")). Agreed that ideally I think it would be more robust for us eventually move when possible away from the try/except (especially with the generic exception catching).

dbanty · 2021-02-11T18:33:39Z

openapi_python_client/templates/property_templates/union_property.py.jinja

@@ -24,20 +36,25 @@ def _parse_{{ property.python_name }}(data: Any) -> {{ property.get_type_string(
 {{ property.python_name }} = _parse_{{ property.python_name }}({{ source }})
 {% endmacro %}

+{# For now we assume there will be no unions of unions #}


In the future we should probably be unrolling unions while building the properties. I don't imagine that would be too difficult, just a check on each inner_property to see if it's a UnionProperty.

Ah, yeah, that makes sense.

forest-benchling · 2021-03-03T00:50:58Z

Hi @dbanty, sorry for the delay, just got back from vacation. I addressed two of your comments, and the rest I was wondering if we can leave to be follow-ups in order to push this through?

dbanty

Looks good, thanks! Sorry this one took so long to get through

dbanty · 2021-03-13T22:05:24Z

Codecov seems to be having some issues reporting final coverage. Can you pull in main to make the checks re-run?

forest-benchling · 2021-03-15T14:16:05Z

@dbanty Merged in main and added another unit test, looks like checks pass now!

forest-benchling added 5 commits February 9, 2021 17:24

Fix tests

4e30205

Manually apply diff

fda721a

Regenerate tests

10926f1

Fix tests

465943d

Fix tests

9ec7394

forest-benchling marked this pull request as ready for review February 9, 2021 23:08

forest-benchling added 3 commits February 9, 2021 18:14

Remove unnecessary check

db33752

Address union of one type

49185f0

Fix union of inline models

b71f718

forest-benchling mentioned this pull request Feb 10, 2021

Fix oneOf of inlined models #335

Closed

Fix union

c251560

dbanty requested changes Feb 10, 2021

View reviewed changes

This was referenced Feb 10, 2021

fix: Fix deserialization of non-required properties #334

Merged

Serialize model query params #316

Merged

forest-benchling added 4 commits February 10, 2021 17:43

Merge upstream/main

0d79ce8

More tests

3c536f7

Change parse annotation to object

8a72732

Address suggestions

b5fa15d

dbanty reviewed Feb 10, 2021

View reviewed changes

forest-benchling added 5 commits February 11, 2021 09:03

Make inner types into a set, fix List type string, add tests

43969ae

Merge upstream/main

2c4fdf8

Add more E2E tests

caea77a

Merge upstream/main

da1d970

Merge openapi-generators#335

682c5ab

forest-benchling requested a review from dbanty February 11, 2021 14:17

dbanty requested changes Feb 11, 2021

View reviewed changes

forest-benchling added 3 commits March 2, 2021 19:35

Merge remote-tracking branch 'upstream/main' into forest-union-3

39faebf

Add tests

55496e5

Change item to type

1ddd486

forest-benchling requested a review from dbanty March 3, 2021 00:50

joshzana mentioned this pull request Mar 3, 2021

New behavior around optional nested objects breaks deserialization when explicit null is returned #343

Closed

dbanty previously approved these changes Mar 13, 2021

View reviewed changes

forest-benchling added 2 commits March 15, 2021 09:37

Merge remote-tracking branch 'upstream/main' into forest-union-3

8bc76af

Add another unit test

0d90970

forest-benchling dismissed dbanty’s stale review via 0d90970 March 15, 2021 14:02

dbanty approved these changes Mar 15, 2021

View reviewed changes

dbanty merged commit f9f0963 into openapi-generators:main Mar 15, 2021

eli-bl deleted the forest-union-3 branch November 26, 2024 22:44

	{% macro check_type_for_construct(property, source) %}{% if property.value_type == str %}isinstance({{ source }}, str){% else %}isinstance({{ source }}, int){% endif %}{% endmacro %}
	{% macro check_type_for_construct(property, source) %}isinstance({{ source }}, {{ property.value_type }}){% endmacro %}

		@@ -11,6 +11,9 @@ class Unset:

		UNSET: Unset = Unset()

		# Used as `FileProperty._json_type_string`

	# Used as `FileProperty._json_type_string`
	{# Used as `FileProperty._json_type_string` #}

	return f"Union[{', '.join(inner_types)}]"
	return f"Union[{', '.join(sorted(inner_types))}]"

Fix deserialization of unions #332

Fix deserialization of unions #332

Conversation

forest-benchling commented Feb 9, 2021 • edited Loading

forest-benchling commented Feb 9, 2021

codecov bot commented Feb 9, 2021 • edited Loading

Codecov Report

dbanty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forest-benchling Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forest-benchling commented Feb 10, 2021

Choose a reason for hiding this comment

forest-benchling commented Feb 11, 2021

dbanty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forest-benchling Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forest-benchling commented Mar 3, 2021

dbanty left a comment

Choose a reason for hiding this comment

dbanty commented Mar 13, 2021

forest-benchling commented Mar 15, 2021

forest-benchling commented Feb 9, 2021 •

edited

Loading

codecov bot commented Feb 9, 2021 •

edited

Loading

forest-benchling Feb 10, 2021 •

edited

Loading

forest-benchling Mar 3, 2021 •

edited

Loading